fix(types): export structured error details - #1082
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| ] | ||
|
|
||
| for type_name in error_detail_types: | ||
| assert getattr(adcp, type_name) is getattr(types, type_name) |
There was a problem hiding this comment.
MUST FIX: This assertion resolves every name via getattr(adcp, ...) on the root package, but the PR only exports these from adcp.types — src/adcp/__init__.py is untouched. Root resolution goes solely through _LAZY (built from the static _LAZY_MODULES) plus submodule passthrough; there is no fallback to adcp.types. Of the 18 names in error_detail_types, only AuthorizationRequiredDetails is in root _LAZY_MODULES. Every other name — including the pre-existing UnsupportedRefinementDimensionDetails and all 17 new ones — raises AttributeError here, so this test fails deterministically. The PR body's "and the package root" is also unmet: from adcp import ActionNotAllowedDetails raises ImportError. Fix: add the 17 names to _LAZY_MODULES["adcp.types"], root __all__, the TYPE_CHECKING block, and the snapshot's adcp section — matching the AuthorizationRequiredDetails precedent.
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
- tests/test_public_api.py:143 — New test asserts a root-package (
adcp) export that the PR never wires;src/adcp/__init__.pyis not updated, so the test fails deterministically.
The PR correctly re-exports the 17 structured error-detail models through the adcp.types facade (types/__init__.py, _eager.py) with matching snapshot, but the stated "package root" export is not delivered. Either wire the re-export into src/adcp/__init__.py or narrow the test to the adcp.types surface actually exported. Row 1 of the decision table applies (a high-severity finding is present).
Blocking findings
- tests/test_public_api.py:143 — New test asserts root-package export the PR never wires; fails deterministically
|
Addressed the automated review finding in b53fdc9: the regression now verifies the requested supported |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior blocking finding resolved, no current findings.
This is a subsequent review. The prior run flagged one high finding (tests/test_public_api.py:143 — test asserted a root-adcp export the PR never wired, failing deterministically). The delta in this push fixes exactly that: test_structured_error_details_are_exported drops the root-package import adcp and switches its assertion from getattr(adcp, name) is getattr(types, name) to getattr(types, name).__name__ == name, checking only the adcp.types surface this PR actually adds. All 19 names resolve through the PEP 562 facade (_RESOLVABLE = __all__ | extras, backed by _eager) and each error-detail class is directly named in generated_poc/error_details/**, so .__name__ matches. The prior blocker is gone and the reviewer surfaced no new findings.
Decision-table walk: no critical/high findings (row 1 no) — the prior high was resolved. gated_paths is false (row 2 no). high_risk is true but reasons are (modified) with zero medium findings, so rows 3/4/5 do not fire. Prior decision was request-changes, not escalate, so row 6 does not apply. No no-auto-approve team match (row 7 no). Zero medium findings (row 8 no). Falls through to row 9 — approve. Net improvement over the prior pass.
1 participant
Summary
adcp.typessurfaceActionNotAllowedReason, which is required to constructActionNotAllowedDetailsFixes #1080.
Validation
uv run pytest -q tests/test_lazy_types.py tests/test_import_layering.py tests/test_public_api.pymake lintmake typecheck-all